estimate_means warning/error cut off? "Could not calculate standard errors for contrasts. This can happen when random effects are involved"#595
Conversation
…rrors for contrasts. This can happen when random effects are involved" Fixes #578
Summary of ChangesHello @strengejacke, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where error messages related to standard error calculation were truncated, particularly when random effects were involved. The changes ensure that the full diagnostic message, including potential suggestions for resolution, is displayed to the user. Additionally, internal code formatting has been improved for better maintainability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where a warning/error message about failing to calculate standard errors was being truncated. The fix involves shortening the initial message and conditionally adding more details. The changes are logical and correctly address the problem.
I've added one comment with a suggestion to improve the message formatting by using paste0() instead of c() to construct the message string. This will prevent unintended spaces and ensure newlines are handled correctly. The other changes are stylistic improvements for readability, which are well done.
There was a problem hiding this comment.
Pull request overview
Fixes the incomplete/truncated .check_standard_errors() message seen in estimate_means() by ensuring the warning only suggests follow-up code when the necessary context (by) is available, and by passing resolved by terms into the check.
Changes:
- Reworked
.check_standard_errors()message assembly to avoid printing “You may try following:” without a snippet. - Updated
estimate_means()to pass the resolvedbyattribute into.check_standard_errors(). - Bumped package version and added a NEWS entry describing the fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| R/utils.R | Adjusts warning message construction in .check_standard_errors() so the follow-up suggestion is only appended when applicable. |
| R/estimate_means.R | Passes attributes(means)$by into .check_standard_errors() so the suggested code snippet can be generated. |
| NEWS.md | Documents the truncated-message fix. |
| DESCRIPTION | Version bump for the patch release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!is.null(by_vars)) { | ||
| msg <- c(msg, insight::color_text(code_snippet, "green"), "\n") | ||
| msg <- c( | ||
| paste0(msg, "You may try following:"), |
There was a problem hiding this comment.
The constructed message is missing whitespace between sentences: format_message(..."involved.") is concatenated with "You may try following:" via paste0() without a leading space/newline, so users will see ...involved.You may try following:. Add a separator (e.g., " You may..." or use paste() with sep = " ") so the warning reads correctly.
| paste0(msg, "You may try following:"), | |
| paste0(msg, " You may try following:"), |
| # sanity check - did method return standard errors? | ||
| .check_standard_errors(out = means, model = model, verbose = verbose) | ||
| .check_standard_errors( | ||
| out = means, | ||
| by = attributes(means)$by, | ||
| model = model, | ||
| verbose = verbose | ||
| ) |
There was a problem hiding this comment.
This PR changes the standard-error warning behavior for estimate_means() by passing the resolved by variables into .check_standard_errors(), but there is no test asserting the warning now includes the suggested follow-up code (and no longer ends with a dangling "You may try following:"). Consider adding an expect_message() (or snapshot) for a model that yields SE = NA to prevent regressions in this user-facing output.
Fixes #578